fix(desktop): serialize shared state writes across processes - #718
fix(desktop): serialize shared state writes across processes#718tonyfettes wants to merge 10 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de080ae365
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @fs.rename(temp.to_string(), target) catch { | ||
| error if @async.is_being_cancelled() => raise error | ||
| _ => { | ||
| remove_if_exists(path) |
There was a problem hiding this comment.
Avoid recursively deleting a non-file target
When the target path exists as a directory—for example, a workspace already contains .openseek/worktrees.json/—the initial file-to-directory rename fails and this fallback calls remove_if_exists, which recursively deletes directories (desktop/internal/fsx/fsx.mbt lines 103–110). Since workspace and worktree registry writes now use this helper, an attach or worktree mutation can silently erase everything under that directory instead of reporting that the registry path has the wrong type; restrict the fallback to regular files or only the specific Windows replacement error.
Useful? React with 👍 / 👎.
dda4151 to
bd70ba7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd70ba73ea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let temp = path.dirname().join(path.basename().to_owned() + ".tmp") | ||
| @fs.write_file( | ||
| temp.to_string(), | ||
| content, | ||
| create_mode=CreateOrTruncate, |
There was a problem hiding this comment.
Create the atomic-save temp file without following links
When a workspace already contains a symlink such as .openseek/worktrees.json.tmp pointing to another writable file, creating or updating a worktree reaches this predictable path with CreateOrTruncate, follows the symlink, and truncates the linked file before the rename merely moves the symlink into place. A repository can contain this path before attachment despite the later Git exclusion, so use an exclusively created, unpredictable temporary regular file (and reject links) rather than truncating a fixed name.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de43e82511
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @fs.remove(target) | ||
| @fs.rename(temp.to_string(), target) |
There was a problem hiding this comment.
Shield the Windows replacement sequence from cancellation
On Windows, when replacing an existing file reaches this fallback, cancellation after remove completes but before rename completes leaves the target absent; the outer handler then deletes the only completed temp file. This path is reachable from the unshielded legacy archive migration, so cancelling session_list_archived while it rewrites a nonempty remaining list can permanently delete archived_sessions.txt and prevent those sessions from being retried. Protect the remove-and-rename sequence from cancellation, or use a platform replacement operation that cannot expose this state.
Useful? React with 👍 / 👎.
| @fs.write_file(temp.to_string(), content, create_mode=CreateNew, permission~) | ||
| commit_temp_file(temp, target) catch { |
There was a problem hiding this comment.
Clean up temporary files when the initial write fails
If write_file creates the random CreateNew file and then raises—for example because the task is cancelled during the write or the filesystem returns ENOSPC—the cleanup handler is never entered because it only wraps commit_temp_file. Each retry therefore leaves another unguessable partial file behind, and failed settings writes can leave credential material in these abandoned files. Wrap the write itself in the same shielded cleanup path.
Useful? React with 👍 / 👎.
ac30712 to
39cdb1f
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39cdb1f5cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // renames, so without this the loser of a race reads its own "already | ||
| // gone" as a failed move and writes the id back for the next launch to | ||
| // retry — forever. | ||
| let shared = @fsx.FileLock::acquire(list, Exclusive) |
There was a problem hiding this comment.
Keep legacy migration lock failures best-effort
When archived_sessions.txt is readable but its sidecar cannot be created or locked—for example, the runtime directory became read-only or the filesystem does not support this lock—FileLock::acquire raises here and aborts the entire archived_sessions request. Previously this migration treated filesystem failures as best-effort and still proceeded to list the current archive stores; catch lock-acquisition failures and skip migration so users can still view their archives.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d656de21d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let file = @fs.open( | ||
| lock_file(path).to_string(), | ||
| mode=ReadWrite, | ||
| create_mode=OpenOrCreate, | ||
| ) |
There was a problem hiding this comment.
Refuse symlinks when opening the lock sidecar
When an attached repository already contains .openseek/worktrees.json.lock as a symlink, OpenOrCreate follows it, so a repository can make the desktop create an empty file outside the workspace or lock an attacker-chosen inode. A link to a per-process or concurrently replaced target also lets separate instances hold different locks, defeating the cross-process serialization and permitting the lost registry updates this change is intended to prevent; open the sidecar without following links and verify that it is a regular file.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9068c1912
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -821,8 +831,11 @@ pub async fn remove_worktree( | |||
| } | |||
| worktree_registry_lock.acquire() | |||
| defer worktree_registry_lock.release() | |||
| let shared = @fsx.FileLock::acquire(worktrees_file(dir).to_path(), Exclusive) | |||
There was a problem hiding this comment.
Acquire the file lock before deleting the worktree
When the sidecar cannot be opened or locked—for example, the workspace contains the symlink or non-regular .openseek/worktrees.json.lock that open_lock_file deliberately refuses—this acquisition raises only after git worktree remove has already deleted the checkout. The request therefore reports failure while leaving the registry entry behind (and a forced removal may already have discarded changes); acquire the cross-process lock before the initial registry read and destructive Git operation so lock failures leave the worktree untouched.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f51726dfb3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| (Some(ownership), Some(dir)) if ownership.hands_off() => | ||
| Some(@instance.identity(dir)) |
There was a problem hiding this comment.
Prevent a non-claimant from becoming the primary instance
When two launches overlap during startup, the first process can acquire the heartbeat claim and then be preempted before registering the Proton identity; the second sees Healthy, also enables single_instance, and can register/run first. The claimant then hands off and exits, leaving the surviving primary without a Claim::run heartbeat; if that primary later wedges, subsequent launches see an unlocked heartbeat, classify themselves as Claimed, and hand off to the wedged Proton owner instead of taking the intended fallback path. Ensure only the heartbeat claimant can become the Proton primary, or transfer the claim to whichever process wins the single-instance election.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0964af771
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let now = @env.now() | ||
| // A stamp from the future is a clock that moved backwards, not a wedge. | ||
| guard now > stamped else { return false } | ||
| now - stamped > HeartbeatStaleMs |
There was a problem hiding this comment.
Recheck stale heartbeats before bypassing the election
When a machine resumes after being suspended for more than 60 seconds, this wall-clock comparison can run before the existing host's sleeping heartbeat task is scheduled to restamp. A launch in that window is classified as seeing a wedged owner, so main.mbt omits single_instance and starts a competing host even though the original event loop is healthy; confirm that the stamp remains unchanged for another heartbeat interval before bypassing the election.
Useful? React with 👍 / 👎.
| /// must not exclude each other. It also keeps a development host from | ||
| /// silently swallowing the launch of the installed app. | ||
| pub fn identity(runtime_dir : @pathx.Path) -> String { | ||
| "community.moonbit.proton.openseek-desktop:" + runtime_dir.to_string() |
There was a problem hiding this comment.
Canonicalize the runtime directory used as the identity
When the same existing runtime directory is reached through lexical aliases—for example, a development checkout invoked once through a symlinked parent and once through its physical path—this produces different Proton identities even though both heartbeat paths resolve to the same inode. The processes therefore win separate elections; the second fails claim because the first holds the physical lock, but its on_start handler simply continues without a heartbeat and both hosts run. This is a distinct remaining non-claimant case exposed by the raw path-based identity, so derive the key from the directory's canonical filesystem path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4d8ceda56
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| on_shutdown=fn(_) { }, | ||
| ) | ||
| let app = match instance_identity { | ||
| Some(identity) => app.single_instance(identity) |
There was a problem hiding this comment.
Restrict update relaunch consumption to the primary
When a second launch reaches single_instance after the primary has written the shared relaunch marker but before its run loop exits, the second process hands off and returns from app.run(), then continues into take_relaunch_target at main.mbt:199-200. That consumes the primary's marker and opens the new bundle while the primary still owns the instance identity, so the reopened process is handed off and exits; when the primary finally closes, its marker is gone and the updated app is not relaunched. Track whether this process actually became the primary, and only let that process consume the marker.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 131a1e3211
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @async.protect_from_cancel(() => { | ||
| @fs.remove(target) | ||
| @fs.rename(temp.to_string(), target) |
There was a problem hiding this comment.
Use an atomic Windows replacement for unlocked readers
On Windows, when the first rename cannot overwrite an existing registry or settings file, this fallback removes the target before the second rename. Readers in another process intentionally take no sidecar lock, and registered_workspaces, read_worktrees_unlocked, and load_engine_settings interpret a missing file as an empty registry or default settings, so any read during this window observes false state despite the new atomic-read contract. Cancellation shielding does not hide this gap from other processes; use a platform atomic-replace operation or synchronize those readers.
Useful? React with 👍 / 👎.
| is_primary.val = true | ||
| match runtime_dir { | ||
| Some(dir) => | ||
| if @instance.claim(dir) is Some(held) { |
There was a problem hiding this comment.
Reject primaries that fail to claim the heartbeat
When a launch previously took the stopped-owner path, it omits single_instance; if the old owner exits before on_start, that bypass host can successfully claim and stamp the heartbeat without owning the Proton identity. A later launch then sees the fresh heartbeat, enables single_instance, wins the otherwise-free Proton election, gets None here because the bypass host holds the claim, and nevertheless continues as a second primary with no heartbeat. The fresh evidence after the earlier startup-race fix is this new identity-bypass path; abort or retry/transfer the election when the elected primary cannot claim.
Useful? React with 👍 / 👎.
131a1e3 to
7a3ae6e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a3ae6e722
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // when the primary finally closed, its marker would be gone and the | ||
| // update would never be entered. The nesting is deliberate; a `&&` chain | ||
| // would take the marker while deciding whether it was allowed to. | ||
| if is_primary.val { |
There was a problem hiding this comment.
Gate startup update cleanup on primary status
When another launch is forwarded while the primary is finishing an update, this tail guard does not fully protect the relaunch marker: before app.run() determines that the process is secondary, spawn_host_pumps at lines 171–173 starts SelfUpdate::run, whose startup cleanup_leftovers unconditionally removes the shared relaunch marker (desktop/internal/update/apply.mbt:133-142). The fresh evidence after the earlier marker-consumption fix is this independent startup-cleanup path, which can delete the marker before the primary reaches take_relaunch_target, leaving the updated app swapped on disk but not relaunched; defer that cleanup until on_start confirms this process is primary.
Useful? React with 👍 / 👎.
7a3ae6e to
5c2c08b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c2c08be7c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| tasks.spawn_bg(allow_failure=true, () => { | ||
| state.host_state().sweep_update_leftovers() |
There was a problem hiding this comment.
Wait for the runtime claim before sweeping updates
When a host started through the stopped-owner bypass has claimed the heartbeat but has no Proton single-instance identity, a later launch can win the identity election and reach this hook while hold(dir) is still waiting for that bypass host. Because the cleanup task is spawned independently, it can delete the active host's staging directory or relaunch marker, interrupting an update or preventing the updated bundle from reopening. The fresh evidence after moving cleanup out of the pump is that this final code still runs it before the process has actually acquired the runtime claim; start owner-only cleanup only after that acquisition succeeds.
Useful? React with 👍 / 👎.
The desktop has never been the only writer of its durable state. A TUI launched in a workspace shares that workspace's store by design, a dev build shares the session root with the installed app, and a leftover process from a previous run outlives the launch that spawned it. Every registry mutation nevertheless serialized on an `@async.Mutex`, which orders this process's own tasks and nothing else: two writers could each read the same list and let the later write erase the earlier one. Add the two primitives that were missing and apply them: - `@fsx.FileLock` — an advisory flock on a sidecar `<file>.lock`, held across processes until `release`. A sidecar rather than the data file because an atomic save replaces the target by rename, so a lock taken on the data file would be attached to the inode rename orphans. - `@fsx.write_text_atomic` — temp file plus rename, so a reader that never takes the lock (another tool, an older build) still sees one complete version or the other. Mutations take the exclusive lock and re-read inside it; reads take no lock at all and rely on the write being atomic. That keeps the tolerant "unreadable registry means nothing is attached" behaviour on the read paths, which are everywhere, instead of turning them into error paths. Applied to workspaces.json, worktrees.json, engine-settings.json, and the legacy archived-sessions list. `write_settings_file` now delegates its rename dance to the shared helper rather than keeping a second copy of the Windows fallback. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The retry after a failed rename exists for Windows, which may refuse to replace an existing file. It removed the target unconditionally through `remove_if_exists`, which deletes a directory recursively — so a path occupied by a directory (`.openseek/worktrees.json/`, say) was erased along with everything under it, and the write then reported success. This was inherited from `write_settings_file`, but promoting it into a shared helper spread it to the workspace and worktree registries. Restrict the retry to a regular file at the target and re-raise the original rename failure otherwise, so a wrong path is reported rather than cleared. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The temp file's name was the target's plus `.tmp`, opened with `CreateOrTruncate`. That is safe in the runtime directory but not in a workspace, which is whatever repository the user attached: a repository can carry a symlink at `.openseek/worktrees.json.tmp`, committed long before the attach that adds the git exclusion. Creating a worktree then followed it, truncating whatever it pointed at and writing the registry there, and the rename afterwards moved the link itself into place — so the registry became a symlink aimed at the clobbered file. Name the temp file with random hex and create it with `CreateNew` (O_CREAT|O_EXCL), which refuses any pre-existing name including a symlink. The random suffix is what keeps a fixed name plus O_EXCL from turning one leftover into a permanent write failure; in exchange the file is no longer reused, so every path out of the commit that is not the rename now removes it, under a cancellation shield. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two gaps left by the private-temp-file change: The cleanup handler only wrapped the commit, so a `write_file` that raised after creating the file — a cancelled task, a full disk — left the temp behind. With a fixed name that self-corrected on the next save; with a random one every attempt abandons another file, and for engine-settings.json those files hold a credential. Wrap the write in the same shielded cleanup. The Windows replacement retry removed the target and then renamed. A cancellation between the two left the target absent, and the caller's cleanup then took the temp file as well, destroying the file the call was meant to update. Commit the pair under a cancellation shield. Reachable from `session_list_archived`, whose legacy-archive migration rewrites its remaining list without a shield of its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The temp file was hardened against a link planted at its name; the lock sidecar beside it was not, and it sits in the same directory. An attached repository can ship `.openseek/worktrees.json.lock`, and `OpenOrCreate` follows it twice over: it creates a file wherever the link points, and it locks that inode, so two instances following a link that moved would each hold a lock on a different file and both walk into the registry write the lock exists to serialize. Create the sidecar with `CreateNew`, which never follows a link, and reject anything that is not a regular file by its own lstat on the already-exists path. A link swapped between that check and the open still wins; closing that needs O_NOFOLLOW, which the filesystem API does not expose. The case this does close is a link committed to a repository, which is the one that arrives without an attacker at the keyboard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The registry now saves through `@fsx.write_text_atomic`, which names its temporary file randomly so that a leftover from a crashed run cannot block every later save. That is exactly what this test used to rely on: it put a directory at the guessable `worktrees.json.tmp` and expected the next write to fail against it. It no longer does, and the property the test guards — a failed staging write leaves the previous registry whole — went untested. Make the store directory unwritable instead. That fails the one step the save cannot route around, and restores the mode before reading back. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`SelfUpdate::run` swept update leftovers before entering its download loop, and that sweep removes the relaunch marker. The marker is not this process's to remove: it belongs to whichever host owns the runtime directory, and it exists exactly across the gap between that host applying an update and its run loop returning. Every launch starts the pump, including one that is about to be handed straight to that host — so a second launch during that gap swept the marker away. The bundle was already swapped on disk and nothing was left to say it should reopen, which is the update silently not being entered. Move the sweep out of the pump. The pump keeps only work a page asks for, which a forwarded launch never has; the sweep becomes `HostState::sweep_update_leftovers`, for the host that owns the directory to run once it knows it does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A second launch used to start a rival host over the same durable state. The cross-process locks make that safe rather than harmless, so make it not happen: the launch is handed to whoever already owns the runtime directory, through Proton's single-instance identity. The rule cannot serve one case. An owner that is still alive but no longer running its event loop would accept the hand-off and never act on it — the app would simply refuse to open, with no way out but finding the process by hand. A plain lock cannot tell that apart from a healthy owner, so the owning host stamps a heartbeat from its event loop and a launch that finds a stalled stamp starts alongside it instead. A stamp left by a host that exited is never read at all, because reading one requires the lock to still be held. `@fsx.FileLock::try_acquire` is the probe that asks who holds a lock without waiting on the process being asked about. Two pieces of owner-only work hang off `app_lifecycle(on_start)`, which only the election winner reaches: holding the heartbeat, and the update leftover sweep. `hold` waits rather than gives up when the claim is taken — a host started on the stopped-owner path holds one without ever entering an election, and giving up would leave the process every later launch is handed to with no heartbeat at all. The relaunch tail is nested rather than a `&&` chain, which would take the marker while deciding whether it was allowed to. The two `catch`-alls in `lock.mbt` become `errdefer`: they only cleaned up and re-raised, and the toolchain now flags that shape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
5c2c08b to
6149c45
Compare
`remove` took the cross-process registry lock only after `git worktree remove` had already deleted the checkout. `FileLock::acquire` can raise — the sidecar it opens is inside the attached workspace, and this code deliberately refuses a symlink or non-regular file standing at that name — so a repository carrying one turned a removal into: checkout deleted, registry row left behind, request reported as failed. Under `force`, after the uncommitted work it had already discarded. Take both locks before the deletion, in the order `create` takes them — lifecycle, then registry mutex, then file lock — so a lock this process cannot get stops the operation while everything is still there. The drain stays outside them: it re-enters the engine, which has registry paths of its own, and a mutex that is not reentrant. The re-read after the git work stays. It now covers a smaller window — between the initial lookup and these locks — but a create can still land a row in it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Winning the single-instance election is not yet owning the runtime directory. A host started on the stopped-owner path holds the claim without ever entering an election, so the process that wins the next one can reach `app_lifecycle(on_start)` while `hold` is still waiting that host out. The leftover sweep was spawned beside `hold` rather than after it, and ran during exactly that wait — deleting the staging directory of an update in progress, or the relaunch marker the other host was about to act on. Give `hold` an `on_claimed` continuation and pass the sweep to it. Owning the directory becomes the precondition of the work instead of something the caller hopes has happened by now, and there is no longer a second task that could act on the directory during the wait. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The desktop has never been the only writer of its durable state:
workspaces.mbtsays so in its header — the two are meant to resume each other's conversations),~/.openseekwith the installed app,Every registry mutation nevertheless serialized only on an
@async.Mutex, which orders this process's own tasks and nothing else. Two writers could each read the same list and let the later write silently erase the earlier one. The non-atomicwrite_textalso let a concurrent reader observe a truncated file and read it as "nothing is attached".What this adds
Two primitives in
internal/fsx/lock.mbt:FileLock::acquire(path, kind)/release()— an advisoryflockon a sidecar<file>.lock, honored across processes. A sidecar rather than the data file itself: an atomic save replaces the target by rename, so a lock taken on the data file would be attached to the inode the rename orphans, and two writers would each hold "the lock" on a different file.write_text_atomic(path, content, permission?)— temp file + rename.Shaped as an acquire/release pair rather than a scoped callback because every call site already sits next to
mutex.acquire(); defer mutex.release()— this inserts two lines instead of reindenting the surrounding block into a closure.The convention
Leaving reads unlocked is deliberate. They are everywhere (
registered_workspaces()alone is called from a dozen places) and they are tolerant by contract: an unreadable registry means "nothing attached", not an error. Locking them would convert that into a raising path. It also would not help the readers that matter most — an external tool or an older build never takes our lock, and what actually protects those is the atomic write.Applied to
workspaces.jsonworktrees.json(create and remove; the create holds the lock across thegit worktree addtoo, so another instance cannot pick the samewt-Nafter this one probed it as free)engine-settings.jsonarchived_sessions.txtmigration (probe before locking, so the common "already migrated" launch leaves no sidecar behind)write_settings_filenow delegates to the shared helper instead of keeping a second copy of the Windows rename fallback.Testing
moon check --target native: cleanmoon test --target native: 3343/3343internal/fsx: atomic save leaves no.tmp, an exclusive lock makes the next holder wait, shared locks admit each other, a lock can be retaken after release@fs.File::lockisflock, which binds the open file description rather than the process, so two acquires of one path contend inside a single process through exactly the mechanism a second instance would hit — the tests cover the cross-process path without spawning a helper.Note on a pre-existing flake
The first full-suite run failed one test,
package/internal/packaging→ "Proton CEF root follows the active runtime manifest", withmkdir target: File exists. A second full run passed 3343/3343. That package's ownensure_diris check-then-mkdir, and two packaging tests race to createdesktop/targetwhen it does not yet exist. Pre-existing, untouched by this change, and left alone as out of scope — but it is the same bug class, if someone wants it.🤖 Generated with Claude Code